Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Revert PR 449 (LP: #2078009) #518

Merged
merged 2 commits into from
Sep 23, 2024
Merged

Revert PR 449 (LP: #2078009) #518

merged 2 commits into from
Sep 23, 2024

Conversation

daniloegea
Copy link
Collaborator

@daniloegea daniloegea commented Sep 16, 2024 •
edited by slyon
Loading

Description

This PR essentially reverts #449

The only conflict was due to an import that was removed (netifaces).

On #449 netplan apply was changed to only apply if there were changes in the backend config files.

While this is a nice approach to avoid unnecessary network disruptions, backend configuration can be generated without apply be aware of it (via the generator (such as when daemon-reload is called) or netplan generate).

In this cases (if the generator or netplan generate runs first), when apply compares the previous configuration with the new one, there will be no changes.

See LP: #2078009. Here, when a new interface is added to the container, a daemon-reload happens (twice apparently). Because of that, the netplan generator is called, new backend configuration is generated and netplan apply wouldn't do any action after that.

Journal from the container at the moment the interface is plugged in:

Sep 16 12:59:22 doh systemd-networkd[441]: veth28d5cf88: Interface name change detected, renamed to eth2.
Sep 16 12:59:22 doh systemd[1]: Starting cloud-init-hotplugd.service - Cloud-init: Hotplug Hook...
Sep 16 12:59:22 doh cloud-init-hotplugd[712]: args=--subsystem=net handle --devpath=/devices/virtual/net/veth28d5cf88 --udevaction=add
Sep 16 12:59:23 doh systemd[1]: Reload requested from client PID 719 ('systemctl') (unit cloud-init-hotplugd.service)...
Sep 16 12:59:23 doh systemd[1]: Reloading...
Sep 16 12:59:23 doh systemd[1]: Configuration file /run/systemd/system/systemd-networkd-wait-online.service.d/10-netplan.conf is marked world-inaccessible. This has no effect as configuration data is accessible via APIs without restrictions. Proceeding anyway.
Sep 16 12:59:23 doh systemd[1]: Configuration file /run/systemd/system/netplan-ovs-cleanup.service is marked world-inaccessible. This has no effect as configuration data is accessible via APIs without restrictions. Proceeding anyway.
Sep 16 12:59:23 doh systemd[1]: Reloading finished in 114 ms.
Sep 16 12:59:23 doh systemd[1]: Reload requested from client PID 769 ('systemctl') (unit cloud-init-hotplugd.service)...
Sep 16 12:59:23 doh systemd[1]: Reloading...
Sep 16 12:59:23 doh systemd[1]: Configuration file /run/systemd/system/systemd-networkd-wait-online.service.d/10-netplan.conf is marked world-inaccessible. This has no effect as configuration data is accessible via APIs without restrictions. Proceeding anyway.
Sep 16 12:59:23 doh systemd[1]: Configuration file /run/systemd/system/netplan-ovs-cleanup.service is marked world-inaccessible. This has no effect as configuration data is accessible via APIs without restrictions. Proceeding anyway.
Sep 16 12:59:23 doh systemd[1]: Reloading finished in 106 ms.
Sep 16 12:59:23 doh systemd[1]: cloud-init-hotplugd.service: Deactivated successfully.
Sep 16 12:59:23 doh systemd[1]: Finished cloud-init-hotplugd.service - Cloud-init: Hotplug Hook.
Sep 16 12:59:33 doh systemd-journald[57]: Forwarding to syslog missed 2162 messages.
Sep 16 12:59:33 doh systemd[1]: systemd-timedated.service: Deactivated successfully.
Sep 16 12:59:33 doh systemd[1]: systemd-hostnamed.service: Deactivated successfully.

I created a PPA with these patches for testing: https://launchpad.net/~danilogondolfo/+archive/ubuntu/bugfixes

Autopkgtests on this PPA.

https://autopkgtest.ubuntu.com/results/autopkgtest-oracular-danilogondolfo-bugfixes/oracular/arm64/n/netplan.io/20240916_144524_6b1ba@/log.gz
https://autopkgtest.ubuntu.com/results/autopkgtest-oracular-danilogondolfo-bugfixes/oracular/ppc64el/n/netplan.io/20240916_144805_5f233@/log.gz
https://autopkgtest.ubuntu.com/results/autopkgtest-oracular-danilogondolfo-bugfixes/oracular/s390x/n/netplan.io/20240916_142627_dc92a@/log.gz
https://autopkgtest.ubuntu.com/results/autopkgtest-oracular-danilogondolfo-bugfixes/oracular/armhf/n/netplan.io/20240916_145550_526ff@/log.gz
https://autopkgtest.ubuntu.com/results/autopkgtest-oracular-danilogondolfo-bugfixes/oracular/amd64/n/netplan.io/20240916_145623_713d6@/log.gz

Checklist

  • Runs make check successfully.
  • Retains code coverage (make check-coverage).
  • New/changed keys in YAML format are documented.
  • (Optional) Adds example YAML for new feature.
  • (Optional) Closes an open bug in Launchpad.

@slyon
Copy link
Collaborator

slyon commented Sep 17, 2024

CC @alfonsosanchezbeato would this revert break any of your usecases?

slyon
slyon approved these changes Sep 17, 2024
View reviewed changes
Copy link
Collaborator

@slyon slyon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. It matches the code from before #449 except for cases where it evolved over time, such as the replacement for netifaces and unused is_composite_member method, in addition to the new udevadm control --reload & udevadm trigger --action=move --subsystem-match=net --settle handling.

Let's give Alfonso some time to chime in, before merging.

@daniloegea daniloegea mentioned this pull request Sep 17, 2024
Draft
5 tasks
@alfonsosanchezbeato
Copy link
Member

alfonsosanchezbeato commented Sep 17, 2024 •
edited
Loading

I'm fine with this, but just to double check, will this restart/reload config for networkd/network-manager as a minimum in the same cases as before? We have had many issues related to this in the past and I'd like to make sure that things like moving from some config to no config, etc. work as expected.

@daniloegea
Copy link
Collaborator Author

Yes, it's essentially reverting the behavior to what it was before until we come up with a better solution. netplan apply will not work properly in some situations because of this.

I think you have a local patch that will restart networkd? I suppose it should continue to work.

@alfonsosanchezbeato
Copy link
Member

Yes, it's essentially reverting the behavior to what it was before until we come up with a better solution. netplan apply will not work properly in some situations because of this.

I think you have a local patch that will restart networkd? I suppose it should continue to work.

I have a patch for core22, but not for core24 where some networkd bug was not present anymore.

@daniloegea
Copy link
Collaborator Author

I think it will not change things for you then. I still think we should have a command line parameter to force the restart of backends so wouldn't need to maintain a patch for that. We probably should consider adding one.

@daniloegea daniloegea merged commit 1ab755c into canonical:main Sep 23, 2024
16 checks passed
@slyon slyon added the stable Might be merged in a stable branch label Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slyon slyon slyon approved these changes

Assignees
No one assigned
Labels
stable Might be merged in a stable branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@daniloegea @slyon @alfonsosanchezbeato